✨ commands #10
✨ commands #10
Conversation
WalkthroughThis update extends a Laravel-based access management system by enhancing the service provider and introducing new console commands. The service provider now registers additional commands and listens for stub publishing events. New command classes for generating control and perimeter classes have been added, alongside corresponding stub templates for scaffolding. Policy methods for authorization checks have been expanded, and the test suite has been updated with new tests and namespace adjustments to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant ASP as AccessServiceProvider
participant App as Application Container
participant Event as Stub Publishing Listener
ASP->>App: registerCommands($devCommands)
loop For each command in $devCommands
App-->>ASP: Command registered (singleton/method)
end
ASP->>Event: registerStubs() listens for PublishingStubs event
Event-->>ASP: Receives stub mappings for registration
sequenceDiagram
participant U as User
participant CMD as ControlMakeCommand
participant FS as FileSystem
U->>CMD: Execute "make:control" command
CMD->>CMD: Call getStub() and resolveStubPath() to retrieve stub
CMD->>CMD: Build class via buildClass() and buildPerimetersReplacements()
CMD->>U: Prompt for missing arguments (if any)
CMD->>FS: Write generated control class file
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
tests/Feature/ControlsShouldTest.php (1)
143-153:⚠️ Potential issueTest Expectation Mismatch
The test methodtest_control_should_not_delete_global_using_shared_overlayed_perimeterimplies that the delete action should be disallowed; however, the assertion currently expects a truthy response. Please verify whether the intended behavior is to have deletion rejected (in which case the assertion should beassertFalse) or if the test name should be revised to accurately describe the expected outcome.
🧹 Nitpick comments (8)
src/Console/stubs/control.stub (1)
15-15: Update Perimeter return type hintThe docblock needs the full namespace for the Perimeter class to be properly documented.
- * @return array<Perimeter> An array of Perimeter objects. + * @return array<\Lomkit\Access\Perimeters\Perimeter> An array of Perimeter objects.src/Policies/ControlledPolicy.php (2)
10-10: Update TODO comment to reflect implemented methodsThe TODO comment needs to be updated since the
restoreandforce_deletemethods have now been implemented.- //@TODO: what to do for other methods like attach / restore / force_delete ? It only has view / etc basic methods + //@TODO: what to do for other methods like attach? Basic methods (view, create, update, delete, restore, forceDelete) are implemented
108-108: Fix docblock comment for restore methodThe docblock description for the
restoremethod contains a copy-paste error. It references deletion instead of restoration.- * @return bool True if deletion is permitted, false otherwise. + * @return bool True if restoration is permitted, false otherwise.tests/Unit/Console/MakeCommandsTest.php (2)
11-11: Use filesystem operations more safelyThe tests use
@unlink()to suppress warnings if files don't exist, which isn't ideal for test maintainability. Consider using a more robust approach for file management in tests.- @unlink(app_path('Access/Perimeters/Perimeter.php')); + if (file_exists(app_path('Access/Perimeters/Perimeter.php'))) { + unlink(app_path('Access/Perimeters/Perimeter.php')); + }Or create a helper method to handle this operation:
private function safelyDeleteFile(string $path): void { if (file_exists($path)) { unlink($path); } }And then use it throughout the test class.
Also applies to: 26-26, 41-41, 56-58, 75-77
1-97: Enhance test coverage with additional assertionsThe test methods could be enhanced with more specific assertions about the structure and content of the generated files.
For example, in addition to checking for the class name and inheritance, you could:
- Verify that the namespace is correct
- Check for the presence of method definitions
- Ensure imports are correctly added
- Validate the formatting of the generated file
This would provide more comprehensive test coverage for the command functionality.
src/Console/PerimeterMakeCommand.php (2)
40-51: Good stub selection logic with improvement opportunity.The stub selection logic works well with the nullish coalescing operator. However, consider adding support for different stub types based on a wider range of options or command context.
119-122: Empty implementation ofafterPromptingForMissingArguments.This method is currently empty with just a comment placeholder. Consider implementing interactive prompting similar to the
ControlMakeCommandclass or remove the method if it's not needed.For example, you could prompt the user to confirm whether they want to create an overlay perimeter:
protected function afterPromptingForMissingArguments(InputInterface $input, OutputInterface $output) { - // + if ($this->didReceiveOptions($input)) { + return; + } + + if (confirm('Should this perimeter be an overlay?', false)) { + $input->setOption('overlay', true); + } }src/Console/ControlMakeCommand.php (1)
84-107: Check for base control could be more robust.The current approach checks for the existence of a base Control class and adjusts namespace imports accordingly. This is a good pattern but could be made more resilient.
Consider enhancing the check:
protected function buildClass($name) { $rootNamespace = $this->rootNamespace(); $controlNamespace = $this->getNamespace($name); $replace = []; - $baseControlExists = file_exists($this->getPath("{$rootNamespace}Access\Controls\Control")); + // Check multiple potential locations for the Control class + $baseControlExists = false; + $controlPaths = [ + $this->getPath("{$rootNamespace}Access\Controls\Control"), + $this->getPath("Lomkit\Access\Controls\Control"), + ]; + + foreach ($controlPaths as $path) { + if (file_exists($path)) { + $baseControlExists = true; + break; + } + } $replace = $this->buildPerimetersReplacements($replace, $this->option('perimeters')); if ($baseControlExists) { $replace["use {$controlNamespace}\Control;\n"] = ''; } else { $replace[' extends Control'] = ''; $replace["use {$rootNamespace}Access\Controls\Control;\n"] = ''; } return str_replace( array_keys($replace), array_values($replace), parent::buildClass($name) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/AccessServiceProvider.php(3 hunks)src/Console/ControlMakeCommand.php(1 hunks)src/Console/PerimeterMakeCommand.php(1 hunks)src/Console/stubs/control.stub(1 hunks)src/Console/stubs/perimeter.overlay.stub(1 hunks)src/Console/stubs/perimeter.plain.stub(1 hunks)src/Policies/ControlledPolicy.php(2 hunks)tests/Feature/ControlsQueryTest.php(1 hunks)tests/Feature/ControlsShouldTest.php(1 hunks)tests/Unit/Console/MakeCommandsTest.php(1 hunks)tests/Unit/StubsTest.php(1 hunks)tests/Unit/TestCase.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/Unit/StubsTest.php (1)
tests/Unit/TestCase.php (1)
TestCase(8-21)
tests/Unit/Console/MakeCommandsTest.php (1)
tests/Unit/TestCase.php (1)
TestCase(8-21)
src/Console/PerimeterMakeCommand.php (1)
src/Console/ControlMakeCommand.php (7)
AsCommand(15-195)getStub(44-47)resolveStubPath(56-61)getDefaultNamespace(70-73)buildClass(84-107)getOptions(150-155)afterPromptingForMissingArguments(165-178)
🪛 GitHub Actions: tests
tests/Unit/StubsTest.php
[error] 13-13: Failed asserting that two arrays are equal. Expected paths do not match actual paths in the test.
🔇 Additional comments (10)
tests/Feature/ControlsQueryTest.php (1)
3-4: Namespace Declaration Update
The addition of the namespaceLomkit\Access\Tests\Featurealigns the test class with the project’s organizational structure. Please verify that the autoloader configuration (e.g., in composer.json) correctly maps this namespace to the proper directory.tests/Unit/TestCase.php (1)
3-4: Namespace Update
The new namespaceLomkit\Access\Tests\Unitappropriately categorizes the unit tests and supports better organization—especially since this class now serves as a base for tests such as those inMakeCommandsTest.src/Console/stubs/perimeter.plain.stub (1)
1-12: Perimeter Plain Stub File Creation
This stub file is designed for generating plain perimeter classes. It correctly uses placeholder syntax (e.g.,{{ namespace }},{{ rootNamespace }}, and{{ class }}) and extends thePerimeterclass. Please ensure that the command responsible for generating perimeter classes (likely thePerimeterMakeCommand) properly replaces these placeholders during scaffolding.tests/Feature/ControlsShouldTest.php (1)
3-4: Namespace Declaration Update
The inclusion of theLomkit\Access\Tests\Featurenamespace ensures consistency with the organizational structure applied to feature tests.src/Console/stubs/perimeter.overlay.stub (1)
1-12: Perimeter Overlay Stub File Creation
The new stub file for overlay perimeters is well defined: it uses the appropriate placeholder variables and extendsOverlayPerimeter. Ensure that the runtime replacement of placeholders via thePerimeterMakeCommandexecutes correctly, thereby generating a functional perimeter class.src/AccessServiceProvider.php (4)
13-16: Well-structured command registration.The array of developer commands is clearly defined and follows good naming conventions. Using an associative array makes the code more maintainable by mapping command names to their respective classes.
52-65: Good implementation of command registration.The
registerCommandsmethod follows Laravel's best practices by:
- Supporting custom registration methods for complex commands
- Falling back to singleton registration for simpler commands
- Properly registering all commands with the application
This implementation allows for flexible command registration while maintaining clean code.
70-77: Well-implemented stub registration.Using the Laravel event system to listen for
PublishingStubsis an elegant way to extend Laravel's stub publishing feature. The use ofrealpath()ensures that symbolic links are properly resolved.
98-101: Correctly implementedprovidesmethod.The
providesmethod properly returns the services provided by this service provider, following Laravel's ServiceProvider pattern.src/Console/PerimeterMakeCommand.php (1)
88-97: Empty replacement array inbuildClass.The
buildClassmethod creates an empty$replacearray which has no effect on the string replacement. This suggests the implementation may be incomplete or unnecessary.If no custom replacements are needed, consider simplifying the method:
-protected function buildClass($name) -{ - $replace = []; - - return str_replace( - array_keys($replace), - array_values($replace), - parent::buildClass($name) - ); -} +protected function buildClass($name) +{ + return parent::buildClass($name); +}Or if replacements are needed, implement them like in the
ControlMakeCommandclass.
| ->run(); | ||
|
|
||
| $this->assertFileExists(app_path('Access/Perimeters/Perimeter.php')); | ||
| $this->assertStringContainsString('class Perimeter extends Perimeter', file_get_contents(app_path('Access/Perimeters/Perimeter.php'))); |
There was a problem hiding this comment.
Fix class inheritance assertion
A class can't extend itself. This assertion appears to be incorrect as it checks for class Perimeter extends Perimeter.
- $this->assertStringContainsString('class Perimeter extends Perimeter', file_get_contents(app_path('Access/Perimeters/Perimeter.php')));
+ $this->assertStringContainsString('class Perimeter extends \Lomkit\Access\Perimeters\Perimeter', file_get_contents(app_path('Access/Perimeters/Perimeter.php')));Alternatively, check what the actual output is and correct the assertion accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertStringContainsString('class Perimeter extends Perimeter', file_get_contents(app_path('Access/Perimeters/Perimeter.php'))); | |
| $this->assertStringContainsString('class Perimeter extends \Lomkit\Access\Perimeters\Perimeter', file_get_contents(app_path('Access/Perimeters/Perimeter.php'))); |
| protected function buildPerimetersReplacements(array $replace, array $perimeters) | ||
| { | ||
| $perimetersImplementation = ''; | ||
|
|
||
| foreach ($perimeters as $perimeter) { | ||
| $perimeterClass = app_path('Access/Perimeters/'.$perimeter.'.php'); | ||
|
|
||
| $perimetersImplementation .= <<<PERIMETER | ||
| \\n | ||
| $perimeterClass::new() | ||
| ->should(function (Model \$user, string \$method, Model \$model) { | ||
| return true; | ||
| }) | ||
| ->allowed(function (Model \$user) { | ||
| return true; | ||
| }) | ||
| ->query(function (Builder \$query, Model \$user) { | ||
| return \$query; | ||
| }),\\n | ||
| PERIMETER; | ||
| } | ||
|
|
||
| return array_merge($replace, [ | ||
| '{{ perimeters }}' => $perimetersImplementation, | ||
| '{{perimeters}}' => $perimetersImplementation, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Perimeter implementation assumptions need refinement.
The buildPerimetersReplacements method makes several assumptions that could lead to issues:
- It hardcodes the path to perimeter classes using
app_path('Access/Perimeters/'). - It assumes specific method signatures with
ModelandBuildertypes without importing them. - It generates stub code with default implementations that always return
trueor the unmodified query.
Consider these improvements:
protected function buildPerimetersReplacements(array $replace, array $perimeters)
{
$perimetersImplementation = '';
+ $rootNamespace = $this->rootNamespace();
foreach ($perimeters as $perimeter) {
- $perimeterClass = app_path('Access/Perimeters/'.$perimeter.'.php');
+ $perimeterClass = $rootNamespace.'Access\\Perimeters\\'.$perimeter;
$perimetersImplementation .= <<<PERIMETER
\n
$perimeterClass::new()
->should(function (\Illuminate\Database\Eloquent\Model \$user, string \$method, \Illuminate\Database\Eloquent\Model \$model) {
return true;
})
->allowed(function (\Illuminate\Database\Eloquent\Model \$user) {
return true;
})
->query(function (\Illuminate\Database\Eloquent\Builder \$query, \Illuminate\Database\Eloquent\Model \$user) {
return \$query;
}),\n
PERIMETER;
}
return array_merge($replace, [
'{{ perimeters }}' => $perimetersImplementation,
'{{perimeters}}' => $perimetersImplementation,
]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function buildPerimetersReplacements(array $replace, array $perimeters) | |
| { | |
| $perimetersImplementation = ''; | |
| foreach ($perimeters as $perimeter) { | |
| $perimeterClass = app_path('Access/Perimeters/'.$perimeter.'.php'); | |
| $perimetersImplementation .= <<<PERIMETER | |
| \\n | |
| $perimeterClass::new() | |
| ->should(function (Model \$user, string \$method, Model \$model) { | |
| return true; | |
| }) | |
| ->allowed(function (Model \$user) { | |
| return true; | |
| }) | |
| ->query(function (Builder \$query, Model \$user) { | |
| return \$query; | |
| }),\\n | |
| PERIMETER; | |
| } | |
| return array_merge($replace, [ | |
| '{{ perimeters }}' => $perimetersImplementation, | |
| '{{perimeters}}' => $perimetersImplementation, | |
| ]); | |
| } | |
| protected function buildPerimetersReplacements(array $replace, array $perimeters) | |
| { | |
| $perimetersImplementation = ''; | |
| $rootNamespace = $this->rootNamespace(); | |
| foreach ($perimeters as $perimeter) { | |
| $perimeterClass = $rootNamespace.'Access\\Perimeters\\'.$perimeter; | |
| $perimetersImplementation .= <<<PERIMETER | |
| \n | |
| $perimeterClass::new() | |
| ->should(function (\Illuminate\Database\Eloquent\Model \$user, string \$method, \Illuminate\Database\Eloquent\Model \$model) { | |
| return true; | |
| }) | |
| ->allowed(function (\Illuminate\Database\Eloquent\Model \$user) { | |
| return true; | |
| }) | |
| ->query(function (\Illuminate\Database\Eloquent\Builder \$query, \Illuminate\Database\Eloquent\Model \$user) { | |
| return \$query; | |
| }),\n | |
| PERIMETER; | |
| } | |
| return array_merge($replace, [ | |
| '{{ perimeters }}' => $perimetersImplementation, | |
| '{{perimeters}}' => $perimetersImplementation, | |
| ]); | |
| } |
| protected function possiblePerimeters() | ||
| { | ||
| $perimetersPath = is_dir(app_path('Access/Perimeters')) ? app_path('Access/Perimeters') : app_path(); | ||
|
|
||
| return (new Collection(Finder::create()->files()->depth(0)->in($perimetersPath))) | ||
| ->map(fn ($file) => $file->getBasename('.php')) | ||
| ->sort() | ||
| ->values() | ||
| ->all(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limited approach to finding perimeters.
The possiblePerimeters method only searches for perimeters in a fixed location, which may not work for all project structures.
Consider a more flexible approach:
protected function possiblePerimeters()
{
- $perimetersPath = is_dir(app_path('Access/Perimeters')) ? app_path('Access/Perimeters') : app_path();
+ // Try multiple potential locations
+ $rootNamespace = $this->rootNamespace();
+ $possiblePaths = [
+ app_path('Access/Perimeters'),
+ app_path('Access'),
+ base_path('src/Access/Perimeters'),
+ base_path('src/Access'),
+ ];
+
+ $perimetersPath = app_path();
+ foreach ($possiblePaths as $path) {
+ if (is_dir($path)) {
+ $perimetersPath = $path;
+ break;
+ }
+ }
return (new Collection(Finder::create()->files()->depth(0)->in($perimetersPath)))
->map(fn ($file) => $file->getBasename('.php'))
+ ->filter(function ($filename) use ($perimetersPath) {
+ // Only include files that are likely to be perimeter classes
+ $content = file_get_contents($perimetersPath.'/'.$filename.'.php');
+ return strpos($content, 'Perimeter') !== false;
+ })
->sort()
->values()
->all();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function possiblePerimeters() | |
| { | |
| $perimetersPath = is_dir(app_path('Access/Perimeters')) ? app_path('Access/Perimeters') : app_path(); | |
| return (new Collection(Finder::create()->files()->depth(0)->in($perimetersPath))) | |
| ->map(fn ($file) => $file->getBasename('.php')) | |
| ->sort() | |
| ->values() | |
| ->all(); | |
| } | |
| protected function possiblePerimeters() | |
| { | |
| // Try multiple potential locations | |
| $rootNamespace = $this->rootNamespace(); | |
| $possiblePaths = [ | |
| app_path('Access/Perimeters'), | |
| app_path('Access'), | |
| base_path('src/Access/Perimeters'), | |
| base_path('src/Access'), | |
| ]; | |
| $perimetersPath = app_path(); | |
| foreach ($possiblePaths as $path) { | |
| if (is_dir($path)) { | |
| $perimetersPath = $path; | |
| break; | |
| } | |
| } | |
| return (new Collection(Finder::create()->files()->depth(0)->in($perimetersPath))) | |
| ->map(fn ($file) => $file->getBasename('.php')) | |
| ->filter(function ($filename) use ($perimetersPath) { | |
| // Only include files that are likely to be perimeter classes | |
| $content = file_get_contents($perimetersPath.'/'.$filename.'.php'); | |
| return strpos($content, 'Perimeter') !== false; | |
| }) | |
| ->sort() | |
| ->values() | |
| ->all(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Console/PerimeterMakeCommand.php (2)
90-99: Consider adding class-specific replacements.
Currently,$replaceis empty, resulting in no modifications to the generated class. If future overlays or placeholders are intended, you might enhance this method to apply them.
101-110: Overlay option might be more suitable as a boolean flag.
Since your overlay option is treated purely as a yes/no flag, consider usingInputOption::VALUE_NONEfor a cleaner approach. That way, you can just check$this->option('overlay')without storing a string or null in some cases.src/Console/ControlMakeCommand.php (1)
185-194: Consider a more flexible perimeter search.
Current logic returns all.phpfiles from a single directory. This may mislead or overload the user if the project organizes perimeters differently or has non-perimeter PHP files.src/Policies/ControlledPolicy.php (1)
10-10: Extend coverage for attach/detach and other pending methods.
You have a TODO remark regarding additional model actions. Plan for a consistent approach (likeshould($user, __FUNCTION__, …)), or remove/resolve the TODO if not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Console/ControlMakeCommand.php(1 hunks)src/Console/PerimeterMakeCommand.php(1 hunks)src/Console/stubs/control.stub(1 hunks)src/Controls/Control.php(1 hunks)src/Policies/ControlledPolicy.php(2 hunks)tests/Feature/ControlsShouldTest.php(2 hunks)tests/Unit/Console/MakeCommandsTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Controls/Control.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Console/stubs/control.stub
- tests/Feature/ControlsShouldTest.php
- tests/Unit/Console/MakeCommandsTest.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Policies/ControlledPolicy.php (2)
tests/Support/Models/Model.php (1)
Model(10-23)src/Perimeters/Perimeter.php (1)
should(83-88)
src/Console/ControlMakeCommand.php (3)
src/Console/PerimeterMakeCommand.php (7)
AsCommand(13-131)getStub(42-53)resolveStubPath(62-67)getDefaultNamespace(76-79)buildClass(90-99)getOptions(106-111)afterPromptingForMissingArguments(121-130)src/Controls/Control.php (1)
perimeters(35-38)src/Policies/ControlledPolicy.php (1)
create(71-74)
src/Console/PerimeterMakeCommand.php (1)
src/Console/ControlMakeCommand.php (6)
getStub(44-47)resolveStubPath(56-61)getDefaultNamespace(70-73)buildClass(84-107)getOptions(150-155)afterPromptingForMissingArguments(165-178)
🔇 Additional comments (7)
src/Console/PerimeterMakeCommand.php (3)
42-53: Stub determination logic looks good.
The conditional assignment for$stubis clear, and the fallback to the plain stub is appropriate.
62-67: Path resolution logic is consistent with Laravel standards.
No issues found; the fallback to__DIR__is a common practice.
121-130:❓ Verification inconclusive
Verify the definition of
didReceiveOptions().
This method call isn’t defined here or in the LaravelGeneratorCommand. Ensure it’s implemented or included from a trait so your condition works as intended.
🏁 Script executed:
#!/bin/bash # Search for didReceiveOptions definition in the codebase rg "function didReceiveOptions" -A 5Length of output: 38
Review Verification: Missing Method Implementation for
didReceiveOptionsBased on the executed search, no definition of
didReceiveOptionswas found in the repository. Please verify by manually checking whether this method is provided through inheritance (e.g., via a trait or a parent class) or if it might be dynamically defined. If it turns out that no such definition exists, then an implementation should be added to ensure the condition inafterPromptingForMissingArguments()works as intended.
- Location to review:
src/Console/PerimeterMakeCommand.php(lines 121-130)- Action required: Confirm the source of
didReceiveOptions—if it is meant to be inherited or included, update the code accordingly. Otherwise, add its definition.src/Console/ControlMakeCommand.php (3)
46-47: Use of the control stub is appropriate.
The call toresolveStubPathaligns with established Laravel file resolution patterns.
58-61: Stub path resolution confirmed.
No issues found with determining the path to the stub file.
84-107: Conditional replacement logic is well-structured.
You correctly handle the scenario where the baseControlclass may or may not exist, removing unneededusestatements.src/Policies/ControlledPolicy.php (1)
101-126: New restore and forceDelete methods align with existing pattern.
These methods are consistent with the existing policy methods, callinggetControl()->should()for each action. Implementation looks correct.
| protected function buildPerimetersReplacements(array $replace, array $perimeters) | ||
| { | ||
| $perimetersImplementation = ''; | ||
|
|
||
| foreach ($perimeters as $perimeter) { | ||
| $perimeterClass = $this->rootNamespace().'Access\\Perimeters\\'.$perimeter.'.php'; | ||
|
|
||
| $perimetersImplementation .= <<<PERIMETER | ||
| \\n | ||
| $perimeterClass::new() | ||
| ->should(function (Model \$user, string \$method, Model \$model) { | ||
| return true; | ||
| }) | ||
| ->allowed(function (Model \$user) { | ||
| return true; | ||
| }) | ||
| ->query(function (Builder \$query, Model \$user) { | ||
| return \$query; | ||
| }),\\n | ||
| PERIMETER; | ||
| } | ||
|
|
||
| return array_merge($replace, [ | ||
| '{{ perimeters }}' => $perimetersImplementation, | ||
| '{{perimeters}}' => $perimetersImplementation, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Fix class reference for perimeters and import missing classes.
Including .php in $perimeterClass breaks the fully qualified class name, preventing ::new() calls. This is a critical issue. Also ensure that Model and Builder are imported in your generated stub or code, since they’re used as types within your should() and query() closures.
A suggested fix for the FQCN:
- $perimeterClass = $this->rootNamespace().'Access\\Perimeters\\'.$perimeter.'.php';
+ $perimeterClass = $this->rootNamespace().'Access\\Perimeters\\'.$perimeter;Also, add imports for:
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Builder;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Console/ControlMakeCommand.php (1)
109-143:⚠️ Potential issueImports for Model and Builder are missing in generated classes.
References to
ModelandBuilderhere will cause a class-not-found error in the generated code unless those classes are imported or fully qualified.Possible fix inside this method (to ensure the generated file has the needed imports):
+ $perimetersImplementation = "use Illuminate\\Database\\Eloquent\\Model;" . PHP_EOL . + "use Illuminate\\Database\\Eloquent\\Builder;" . PHP_EOL . + $perimetersImplementation; ... or fully qualify references: - ->should(function (Model $user, string $method, Model $model) { + ->should(function (\Illuminate\Database\Eloquent\Model $user, string $method, \Illuminate\Database\Eloquent\Model $model) {
🧹 Nitpick comments (3)
tests/Unit/Console/MakeCommandsTest.php (2)
9-22: Consider verifying namespace and additional content.While the test verifies the class name and extension, consider also asserting the generated file’s namespace to ensure it matches the expected structure. This can make the test more robust.
73-96: Optionally verify excluded perimeters.While the test confirms that
TestPerimeteris referenced, it might be helpful to also assert thatSecondTestPerimeteris not included when it’s not selected. This would ensure correct handling of multiple user choices.src/Console/ControlMakeCommand.php (1)
180-195: Docblock mismatch and limited search path.
- The docblock still references “model names” though it returns perimeter classes. Update the comment to reflect actual usage.
- Searching exclusively in
app_path('Access/Perimeters')orapp_path()may be too narrow for broader project structures. Consider offering additional search paths or a configuration option for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Console/ControlMakeCommand.php(1 hunks)tests/Unit/Console/MakeCommandsTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/Unit/Console/MakeCommandsTest.php (1)
tests/Unit/TestCase.php (1)
TestCase(8-21)
src/Console/ControlMakeCommand.php (3)
src/Console/PerimeterMakeCommand.php (7)
AsCommand(13-131)getStub(42-53)resolveStubPath(62-67)getDefaultNamespace(76-79)buildClass(90-99)getOptions(106-111)afterPromptingForMissingArguments(121-130)src/Controls/Control.php (1)
perimeters(35-38)src/Policies/ControlledPolicy.php (1)
create(71-74)
🔇 Additional comments (11)
tests/Unit/Console/MakeCommandsTest.php (4)
1-4: Namespace and class imports look good.No issues found. The file structure and namespace declaration appear consistent with the rest of the codebase.
24-37: Overlay perimeter test looks valid.It precisely checks that the generated class extends
OverlayPerimeter. No further issues spotted.
39-52: Straightforward test coverage for make:control command.Validates class generation for
TestControl. The approach is consistent with Laravel’sartisantesting patterns.
54-71: Good check for base control usage.This test ensures that if a
Control.phpfile exists, the newly generated class extendsControl. Implementation and assertions are appropriate.src/Console/ControlMakeCommand.php (7)
15-16: Command attribute usage is correct.
#[AsCommand(name: 'make:control')]cleanly declares the artisan command name. No issues here.
39-48: Stub retrieval matches the existing pattern.The method pulls the
control.stubconsistently with the approach in the perimeter command. No concerns.
49-61: Stub path resolution logic is appropriate.It checks for a custom path before falling back to the default directory. This is a good practice.
63-73: Namespace returning the Controls folder is valid.The default namespace pointing to
\Access\Controlsaligns well with the project structure.
75-108: Handles base control presence gracefully.The logic removes or keeps
extends Controlbased on whether the file exists. Implementation looks sound.
145-155: Perimeter option is registered correctly.The
--perimetersoption is well-defined and accommodates multiple values. No issues noted.
157-178: Interactive prompt logic is straightforward and aligns with Laravel patterns.No problems with how user input is collected and assigned to the command options.
Summary by CodeRabbit
New Features
Bug Fixes
Tests